Pull request overview

This PR adds new experimental connection-level diagnostics and control points to System.Net.Http’s SocketsHttpHandler, including a configurable pooled-connection eviction callback and a connection identifier that’s propagated through connection establishment, request sending, and plaintext stream filtering. It also adds functional coverage across HTTP/1.1, HTTP/2, and HTTP/3 (including Alt-Svc).

Changes:

• Introduces experimental public APIs: SocketsHttpHandler.ShouldEvictConnection, SocketsHttpConnectionEvictionContext, and HttpRequestMessage.ConnectionId (plus ConnectionId surfacing in connect/plaintext-filter contexts).
• Implements eviction evaluation in the connection pool and stamps ConnectionId onto requests for HTTP/1.1, HTTP/2, and HTTP/3.
• Adds functional tests (mostly OuterLoop) exercising eviction behavior and connection id propagation, plus warning suppressions for the experimental diagnostic ID.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Impl. LGTM, didn't look at tests too much

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Holistic Review

Motivation: Well justified. Both new APIs (SocketsHttpHandler.ShouldEvictConnection + SocketsHttpConnectionEvictionContext, and HttpRequestMessage.ConnectionId) are backed by approved proposals (#130102, #130108, both carrying api-approved), and the scenario—evicting healthy pooled connections in response to DNS/address changes without recycling purely via PooledConnectionLifetime—is real and previously required ceremony over EventSource telemetry.

Approach: Reasonable and well-integrated. A single connection id is allocated up front in ConnectAsync and threaded through ConnectCallback, plaintext filter, telemetry, the eviction context, and the request message, giving one consistent correlation id. Eviction runs off the maintenance timer outside the pool lock with snapshots, a generation counter re-evaluates HTTP/1.1 connections missed while busy, and callback exceptions are swallowed so a misbehaving callback can't take down pool maintenance. The XML docs are unusually thorough, especially around the CONNECT-tunnel id distinction. Both approved API shapes match the ref exactly.

Summary: ⚠️ Needs Human Review. The design is sound and the API is approved, but the HttpRequestMessage flag refactor drops the atomic Interlocked.CompareExchange that enforced single-send and newly mutates _flags mid-request (via the ConnectionId setter from each connection's SendAsync), introducing potential races with the non-atomic Disposed/AlreadySent writes. A networking maintainer should confirm whether that relaxation is intentional. Note the PR is marked NO-MERGE pending API review completion, and one maintainer (rzikm) has already approved.

Detailed Findings

⚠️ Thread-safety — HttpRequestMessage flag refactor drops atomicity (see inline comment)

The conversion of the packed int _sendStatus (mutated atomically via Interlocked.CompareExchange/|=) to a plain MessageFlags _flags field removes the atomic claim in MarkAsSent and adds a new mid-request writer through the ConnectionId setter. Previously _flags-equivalent writes after send were effectively single-threaded; now request.ConnectionId = Id runs inside connection SendAsync and can overlap a concurrent Dispose() (_flags |= Disposed) or a racing send, tearing a non-atomic read-modify-write. Detailed inline comment on HttpRequestMessage.cs. Worth a maintainer's explicit confirmation that single-send detection and flag integrity are still guaranteed.

✅ API conformance — matches approved proposals

The ref/ surface (ShouldEvictConnection, SocketsHttpConnectionEvictionContext with Age/ConnectionId/HttpVersion/DnsEndPoint/RemoteEndPoint, and ConnectionId on the connect/plaintext/request contexts) matches the shapes approved in #130102/#130108, including the [Experimental("SYSLIB5008")] diagnostic and nullable IPEndPoint? RemoteEndPoint. list-of-diagnostics.md and Experimentals.cs are updated consistently.

✅ Concurrency of eviction evaluation — reasonable guards

Callback invocation is gated by _evictionEvaluationInProgress (pool-level) and _evictionCallbackInProgress (per-connection) plus a generation counter; snapshots for HTTP/2 and HTTP/3 are taken under SyncObj and the callback is awaited outside the lock. _connectionDisposalCts is canceled (not disposed) on close so in-flight callback tokens stay valid. The Interlocked.Exchange(ref _evictionEvaluationInProgress, true) / _evictionCallbackInProgress uses rely on the generic Interlocked.Exchange<T> over bool, which is valid. The author explicitly notes the benign race of running the callback more than once per generation.

✅ Eviction cadence and lifetime interaction

When a callback is set, the maintenance timer is capped to fire at least every 5s regardless of a large/infinite idle timeout, giving eviction a predictable cadence; CheckExpirationOnGet/OnReturn/IsUsable all newly short-circuit on MarkedForEviction so a marked connection is retired via get/return/scavenge without serving new requests. _markedForEviction is volatile, appropriate for the cross-thread read.

💡 Test coverage — good breadth, timer-dependent

Tests exercise Alt-Svc/HTTP/3 authority reporting, origin-endpoint reporting, and ConnectionId correlation across versions; several are [OuterLoop] because they wait on the pool maintenance timer, which is appropriate. Consider (follow-up, non-blocking) a targeted test asserting single-send / ConnectionId-stamping behavior under concurrent sends to lock in the _flags semantics discussed above.

Generated by Holistic Review · 247.4 AIC · ⌖ 11.4 AIC · ⊞ 10K

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

        internal bool MarkAsSent()
        {
            MessageFlags previousFlags = _flags;
            _flags = previousFlags | MessageFlags.AlreadySent;
            return !previousFlags.HasFlag(MessageFlags.AlreadySent);
        }

            get => _flags.HasFlag(MessageFlags.ConnectionIdSet) ? _connectionId : null;

            get => _flags.HasFlag(MessageFlags.Disposed);

Holistic Review

Motivation: Unchanged and well justified. Both new APIs (SocketsHttpHandler.ShouldEvictConnection + SocketsHttpConnectionEvictionContext, and HttpRequestMessage.ConnectionId) are backed by approved proposals (#130102, #130108), enabling eviction of healthy pooled connections in response to DNS/address changes without relying solely on PooledConnectionLifetime.

Approach: Refined since the last review. The eviction evaluation loop in HttpConnectionPool.EvaluateConnectionsForEviction was changed from an async method that awaited each connection's callback one-by-one to a synchronous method that starts each per-connection check as fire-and-forget (_ = connection.EvaluateForEvictionAsync()). This removes the previous pool-level _evictionEvaluationInProgress guard, which is safe: the maintenance timer (HttpConnectionPoolManager.RemoveStalePools) is single-threaded per manager (a one-shot Timer.Change restarted only at the end of each pass), so passes don't overlap; per-connection overlap is still guarded by _evictionCallbackInProgress via Interlocked.Exchange, and EvaluateForEvictionAsync catches all exceptions internally so the discarded task never faults. The change correctly prevents a single slow/blocking user callback from stalling evaluation of the other pooled connections (and subsequent passes) — a real improvement over the prior serial-await design. The remaining approach (single up-front connection id threaded through connect/plaintext/telemetry/eviction/request, generation-counter re-evaluation of busy HTTP/1.1 connections, callback run outside the pool lock) is unchanged.

Summary: ⚠️ Needs Human Review. The concurrency refinement in the incremental commits is correct and well-commented, and the expanded/restructured tests (protocol-parameterized base class, a new test asserting that a connection whose callback blocks "forever" does not prevent the other pooled connection's callback from running) directly validate the new fire-and-forget semantics. The verdict remains Needs Human Review for the same reason as the initial review, which is unchanged by these commits: the HttpRequestMessage flag refactor (packed int _sendStatus → plain MessageFlags _flags) drops the atomic single-send claim and adds a mid-request _flags writer via the ConnectionId setter, a relaxation a networking maintainer should explicitly confirm. That code lives in earlier commits unchanged by this update, so it is carried forward in this cumulative assessment rather than re-raised as a new finding. The PR also remains marked NO-MERGE pending API-review completion.

Assessment History

• review 4730675022 (commit 8be3be0246ab): verdict was ⚠️ Needs Human Review; current verdict is ⚠️ Needs Human Review. Changed in reasoning while the verdict is unchanged: the initial review flagged the eviction concurrency as "reasonable guards" but noted the pool-level guard; commits 12–14 replaced the serial-await design (and its pool-level _evictionEvaluationInProgress guard) with a fire-and-forget design that better tolerates slow callbacks, so the approach assessment improved. The verdict itself is held solely by the unchanged HttpRequestMessage flag-atomicity concern and the NO-MERGE/API-review status, neither of which these commits touch.

Detailed Findings

No new actionable findings in the incremental changes (commits a4176bb9ca27, eb1e44388f3a, 2dc0efb170dc). The fire-and-forget eviction refactor is correct: the maintenance pass is single-threaded, per-connection overlap remains guarded, and the awaited callback body swallows exceptions so discarding the task is safe. Test changes are limited to expanding/restructuring existing eviction and ConnectionId coverage and read correctly. The previously noted HttpRequestMessage flag-atomicity concern is in code unchanged by these commits and is not re-raised here per incremental-review scope.

Generated by Holistic Review · 119.2 AIC · ⌖ 11.8 AIC · ⊞ 10K